fix: tag stdin with mtime and mode when piping to cli 'add'#2832
Conversation
|
Neat! Could you add some tests around this please? The CLI handlers add take a See the |
|
Hi @achingbrain, I have added in a test to cover the piping with the mtime argument. The build failed shows a failure on the |
| : tagWithMetadata(getStdin(), { | ||
| mode: argv.mode, | ||
| mtime | ||
| }) // Pipe to ipfs.add tagging with mode and mtime |
There was a problem hiding this comment.
Would this work, without having to use normaliseAddInput?:
| : tagWithMetadata(getStdin(), { | |
| mode: argv.mode, | |
| mtime | |
| }) // Pipe to ipfs.add tagging with mode and mtime | |
| : { | |
| content: getStdin(), | |
| mode: argv.mode, | |
| mtime | |
| } // Pipe to ipfs.add tagging with mode and mtime |
There was a problem hiding this comment.
Just gave it a go, but it seems ipfs.add is expecting an iterator:
TypeError: Cannot read property 'Symbol(Symbol.asyncIterator)' of undefined
We are able to get the job done if we turn the object into an async iterator
: (function * () {
yield {
content: getStdin(),
mode: argv.mode,
mtime
}
})()Is there a iterator util for turning an element into a async iterator?
There was a problem hiding this comment.
Weird, that should be supported. You should also be able to do:
: [{
content: getStdin(),
mode: argv.mode,
mtime
}]There was a problem hiding this comment.
That worked. Should I open a bug for the add function when passed:
{
content: getStdin(),
mode: argv.mode,
mtime
}There was a problem hiding this comment.
I think this is because the test stubs ipfs.add to only return something when passed an iterable (async or otherwise) as the first argument:
ipfs.add.withArgs(matchIterable(), defaultAddArgs()).returns([{
cid,
path: 'readme'
}])If you pass:
{
content: getStdin(),
mode: argv.mode,
mtime
}You're not passing an iterable so the stub doesn't match and undefined is returned from ipfs.add because JavaScript which causes the reported TypeError in the for await..of loop.
The test needs to be updated to this:
it('add from pipe with mtime=100', async () => {
const cid = new CID('QmPZ9gcCEpqKTo6aq61g2nXGUhM4iCL3ewB6LDXZCtioEB')
ipfs.add.withArgs(sinon.match({
content: matchIterable(),
mtime: { secs: 100 }
}), defaultAddArgs()).returns([{
cid,
path: 'readme'
}])
const proc = cli('add --mtime=100', {
ipfs,
getStdin: function * () {
yield Buffer.from('hello\n')
}
})
const out = await proc
expect(out).to.equal(`added ${cid} ${cid}\n`)
const input = ipfs.add.getCall(0).args[0]
expect(input).to.have.nested.property('mtime.secs', 100)
})There was a problem hiding this comment.
TBH it could even lose the last assertion (for the mtime.secs property) as it'll only fail if the ipfs.add stub doesn't match, which will cause the TypeError issue before it gets to that assertion.
There was a problem hiding this comment.
@achingbrain I have updated the usage and test as suggested. The mocking had to be changed in 'add from pipe' as well.
ae912a8 to
5c21376
Compare
5c21376 to
4a5c571
Compare
|
Please could you rebase this on top of master or merge master into this branch to resolve the merge conflict? Then once CI is green it should be good to go. |
Converts the stdin stream to a file object and tags it with the mtime and mode. fixes ipfs#2763
4a5c571 to
43bf310
Compare
|
@achingbrain I have rebased off of master. The build has failed on 'interop - electron-renderer', which I suspect is a build gremlin, can you kick off another build? |
Converts the stdin stream to a file object and tags it with the mtime and mode. fixes ipfs#2763
Converts the stdin stream to a async iterator of one file and tags that file with the mtime and
mode.
fixes #2763